-
-
Notifications
You must be signed in to change notification settings - Fork 1
feat: upgrade account to smart account #188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
packages/gator-permissions-snap/test/core/permissionRequestLifecycleOrchestrator.test.ts
Show resolved
Hide resolved
|
Looks good! I think it’d be better to update the docs to include this new feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! A couple minor comments.
| result.upgradedAddress?.toLowerCase() === | ||
| eip7702StatelessDeleGatorImpl.toLowerCase(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the wallet is making this check. If that is the case should we be duplicating the check here?
On the one hand, the result should always be the eip7702StatelessDeleGatorImpl address, so this probably isn't harmful.
On the other hand, the wallet is the arbiter of which account should be upgraded to, so if for some reason the wallet is upgrading to a different account (maybe we deploy an updated version of the Eip7702Stateless contract that is otherwise backwards compatible), the snap will need to be updated also. Presently the wallet receives the contract address to delegate to via over-the-air configuration, so this delegation address could be changed without a software update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wallet is not checking the address to which is upgraded. Also I'm thinking there can be multiple versions of the stateless contract and we would need to know to which contracts its upgraded and suggest and upgrade if its not to the one we need?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a complex problem, because we can't request a specific account, just that the account is upgraded, so it seems counterintuitive that the caller cannot specify the target when upgrading, but must know about the desired address when checking if it's upgraded.
I would expect more like a get_capabilities call, where the caller asks whether the EOA is a MetaMask Smart Account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good.
I still have a concern about how we manage the delegated contract address - but that's a bigger question, which we should probably discuss outside of the context of this PR.
We can either hold off on merging this until that question is resolved, or merge this and revisit once we have a decision (I prefer the former, in the interests of expediency).
Description
If permission is being done by an account that has not been transformed to Smart account then we trigger an account upgrade.
Related issues
Depends on: MetaMask/metamask-extension#36452
Manual testing steps
Screenshots/Recordings
After
Screen.Recording.2025-10-02.at.14.33.28.mov
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Adds smart account upgrade check/trigger to the permission flow with UI warning and supporting chain metadata; includes tests.
getAccountUpgradeStatus(params)andupgradeAccount(params)usingwallet_getAccountUpgradeStatusandwallet_upgradeAccount; wrap failures withInternalError.upgradeAccountif needed before resolving permission.isAccountUpgradedto UI.isAccountUpgradedis false; minorTokenBalanceFieldprop tweak.contracts.eip7702StatelessDeleGatorImpladdress.Written by Cursor Bugbot for commit 49df0d1. This will update automatically on new commits. Configure here.